fix(general): harden media action execution and make notification favorite state episode-aware and remove latency#3583
Conversation
Await custom command handlers before returning SessionResult. Return structured SessionError values for unsupported commands and runtime failures. Switch callback command hooks to suspend handlers to align with async execution.
Serialize like/library/radio/target-playlist actions with dedicated mutexes to keep rapid taps deterministic. Make library toggles optimistic locally and run remote sync in-order to remove perceived delay. Move target-playlist reads/writes to IO and prevent duplicate inserts under repeated actions. Refactor notification command internals to suspend handlers and improve failure reporting behavior.
📝 WalkthroughWalkthroughCallback handlers were converted to suspending functions and onCustomCommand was reworked to run handlers in coroutines with explicit error mapping. MusicService gained mutex-guarded suspend internals for like/library/playlist/radio mutations, optimistic DB updates with rollback on failure, targeted metadata-change detection, and synchronized UI updates. ChangesSession callbacks and MusicService mutations
Sequence Diagram(s)sequenceDiagram
participant Controller as Client/Controller
participant Session as MediaLibrarySessionCallback
participant Service as MusicService
participant DB as Database
participant YouTube as YouTube API
participant UI as Notification/Widget
Controller->>Session: send customAction (e.g., "TOGGLE_LIKE")
Session->>Service: scope.future { invoke suspending handler }
Service->>Service: withLock (acquire Mutex)
Service->>DB: optimistic update (suspend)
alt external sync required
Service->>YouTube: sync like/library/playlist (suspend)
YouTube-->>Service: result / error
end
Service->>DB: refresh/read merged state (suspend)
Service->>Service: mergeSongStateIntoCurrentMetadata
Service->>UI: updateNotification() / updateWidgetUI()
Service-->>Session: complete Future with SessionResult (success or mapped error)
Session-->>Controller: future completed with result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt (1)
1967-1971: Inconsistent error handling compared to the other public wrappers.
toggleLike(),toggleLibrary(), andaddToTargetPlaylist()now log viaTimber.tag(TAG).e(...)andreportException(e)on failure. With the newtoggleStartRadioInternal()throwingIllegalStateExceptionwhen no recommendations are applied, this wrapper will silently swallow the failure viaSilentHandler, leaving no trace in logs/crash reports for user-triggered radio starts.♻️ Align with the other wrappers
fun toggleStartRadio() { scope.launch(SilentHandler) { - toggleStartRadioInternal() + try { + toggleStartRadioInternal() + } catch (e: CancellationException) { + throw e + } catch (e: Exception) { + Timber.tag(TAG).e(e, "Failed to start radio") + reportException(e) + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt` around lines 1967 - 1971, The public wrapper toggleStartRadio should mirror other wrappers (toggleLike, toggleLibrary, addToTargetPlaylist) by catching exceptions from toggleStartRadioInternal and recording them instead of silently swallowing them; update toggleStartRadio to launch the coroutine, wrap the call to toggleStartRadioInternal() in a try/catch, and in the catch block call Timber.tag(TAG).e(e, "Failed to start radio") and invoke reportException(e) so IllegalStateException and other errors are logged and reported.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 1220-1228: The current favorite computation can be incorrectly
overridden by stale metadata; update the logic so that when
currentSong.value?.song is non-null its DB-backed fields take precedence:
determine isFavorite by first checking if song != null — for episodes use
(song.inLibrary != null) and for non-episodes use (song.liked == true); only
when song == null fall back to currentMediaMetadata (metadata?.inLibrary != null
for episodes, metadata?.liked == true for non-episodes). Use the existing
identifiers currentSong, currentMediaMetadata, isEpisode, isFavorite,
song.inLibrary, song.liked, metadata.inLibrary, and metadata.liked when
implementing this precedence change.
- Around line 1951-1960: The current update uses the player's current metadata
(baseMetadata) to set inLibrary/isEpisode which can overwrite metadata for a
newly playing track; in MusicService around the currentMediaMetadata update
(using baseMetadata, player.currentMetadata and songEntity), first verify the
metadata belongs to the same track before mutating UI state: compare the unique
ID of songEntity (or the DB row id used in the toggle) with baseMetadata?.id (or
player.currentMetadata?.id) and only perform currentMediaMetadata.value =
baseMetadata?.copy(...) when the IDs match; if they don't match, skip updating
currentMediaMetadata (but still call updateNotification()/updateWidgetUI as
appropriate) so you don't attribute the toggle to a different track.
---
Nitpick comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 1967-1971: The public wrapper toggleStartRadio should mirror other
wrappers (toggleLike, toggleLibrary, addToTargetPlaylist) by catching exceptions
from toggleStartRadioInternal and recording them instead of silently swallowing
them; update toggleStartRadio to launch the coroutine, wrap the call to
toggleStartRadioInternal() in a try/catch, and in the catch block call
Timber.tag(TAG).e(e, "Failed to start radio") and invoke reportException(e) so
IllegalStateException and other errors are logged and reported.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3ff92f0c-0c20-4956-9a28-0d7ec1dd57e4
📒 Files selected for processing (2)
app/src/main/kotlin/com/metrolist/music/playback/MediaLibrarySessionCallback.ktapp/src/main/kotlin/com/metrolist/music/playback/MusicService.kt
- Prefer DB-backed song state over possibly stale metadata when deriving notification favorite state. - Preserve coroutine cancellation in start-radio fallback path instead of swallowing CancellationException. - Move currentSong capture inside library/like mutexes to avoid stale concurrent toggles. - Re-read persisted song rows after optimistic DB updates and merge liked/likedDate/inLibrary/isEpisode into current metadata before notification/widget refresh. - Add rollback path for toggleLibrary remote failures and refresh UI from rolled-back state for consistency. - Guard episode metadata writeback to current track ID to avoid cross-track attribution during transitions. Validation: ./gradlew :app:compileFossDebugKotlin --no-daemon --console=plain (BUILD SUCCESSFUL).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt (1)
1436-1442: Stale log tag in the new internal method.The warning on Line 1440 still reads
"startRadioSeamlessly called before player initialization", buttoggleStartRadioInternalis now also reachable viatoggleStartRadio()(Line 2015) and from the session callback wiring at Line 510. The message will misattribute the caller when the other entry points are used.- Timber.tag(TAG).w("startRadioSeamlessly called before player initialization") + Timber.tag(TAG).w("toggleStartRadioInternal called before player initialization")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt` around lines 1436 - 1442, The warning message in toggleStartRadioInternal still says "startRadioSeamlessly called before player initialization" which misattributes callers; update the Timber.tag(TAG).w(...) message inside the suspend function toggleStartRadioInternal to a neutral or correct message (e.g., "toggleStartRadioInternal called before player initialization" or "start radio called before player initialization") so it accurately reflects this method can be reached from toggleStartRadio and session callback wiring; modify only the string literal in toggleStartRadioInternal's early-return log.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 1861-1920: toggleLikeInternal currently does an optimistic DB
update then calls syncUtils.likeSong(...) and
DownloadService.sendAddDownload(...) without rollback on failure; mirror
toggleLibraryInternal: wrap the remote calls (syncUtils.likeSong and the
DownloadService call) in a try/catch, and on exception perform an IO re-read and
rollback update to restore the previous song state (use the original songEntity
or the DB row read back), call mergeSongStateIntoCurrentMetadata(rollbackSong),
updateNotification(), updateWidgetUI(player.isPlaying) while still inside the
lock, then rethrow the exception so outer toggleLike() can log/report it.
---
Nitpick comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 1436-1442: The warning message in toggleStartRadioInternal still
says "startRadioSeamlessly called before player initialization" which
misattributes callers; update the Timber.tag(TAG).w(...) message inside the
suspend function toggleStartRadioInternal to a neutral or correct message (e.g.,
"toggleStartRadioInternal called before player initialization" or "start radio
called before player initialization") so it accurately reflects this method can
be reached from toggleStartRadio and session callback wiring; modify only the
string literal in toggleStartRadioInternal's early-return log.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 000743fc-074a-432b-8955-07fc49c05c1c
📒 Files selected for processing (1)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt
- Wrap toggleLike remote sync + auto-download calls in try/catch after optimistic DB updates.\n- Roll back to the pre-toggle song state on non-cancellation failures, then merge metadata and refresh notification/widget UI before rethrowing for centralized error reporting.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt (1)
1459-1536:⚠️ Potential issue | 🟠 MajorOuter catch in
toggleStartRadioInternalstill swallowsCancellationException.The inner fallback catch at Line 1532 correctly rethrows
CancellationException, but the outercatch (e: Exception)at Line 1494 does not. In Kotlin,CancellationExceptionextendsException, so a cancellation raised by the primarywithContext(Dispatchers.IO) { ... }(Lines 1461‑1466) is caught here, the fallback network calls then execute against an already-cancelled coroutine, and the original cancellation is lost if the fallback completes (or its inner catch doesn't see a newCancellationException). This contradicts the PR's "Preserve coroutine cancellation" commit — the fix needs to apply at both levels.🛡️ Suggested fix
} catch (e: Exception) { + if (e is CancellationException) throw e // Fallback: try with related endpoint try {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt` around lines 1459 - 1536, The outer catch in toggleStartRadioInternal around the primary withContext block swallows CancellationException; update that catch (e: Exception) to rethrow CancellationException (e.g., if (e is CancellationException) throw e) before handling/logging fallback so coroutine cancellation is preserved, similar to the inner fallback catch, and ensure the same pattern is applied to any other broad Exception catches in toggleStartRadioInternal that wrap suspend calls like withContext(Dispatchers.IO) { ... } and radioQueue.getInitialStatus().
🧹 Nitpick comments (1)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt (1)
1844-1846: Nit: stray trailing brace formatting.The
} }on Line 1845 is syntactically fine but visually confusing. Consider splitting onto separate lines to match the style oftoggleLikeInternal/addToTargetPlaylistInternal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt` around lines 1844 - 1846, The closing braces after the catch rethrow in MusicService.kt are squashed ("} }") and should be split onto separate lines to match the project's brace style; locate the block that contains the `throw e` (within the same method that parallels `toggleLikeInternal`/`addToTargetPlaylistInternal`) and place each closing brace on its own line so the method and enclosing block are clearly separated and visually consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 1874-1878: The episode "save for later" path needs the same
rollback parity as other toggles: in toggleEpisodeSaveForLater (which
toggleLikeInternal delegates to), capture the previous inLibrary/save-for-later
state before performing the optimistic DB update and mutating
currentMediaMetadata, then wrap the remote call to syncUtils.saveEpisode(...) in
a try/catch; on non-cancellation failures restore the DB row to the previous
state, re-merge the previous metadata into currentMediaMetadata, trigger the
notification/widget refresh/update functions (same ones used by
toggleLibraryInternal/toggleLikeInternal rollback), and rethrow the exception;
apply this pattern to the other occurrence of toggleEpisodeSaveForLater logic
around lines 1998-2031 as well so both paths have symmetric rollback behavior.
---
Outside diff comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 1459-1536: The outer catch in toggleStartRadioInternal around the
primary withContext block swallows CancellationException; update that catch (e:
Exception) to rethrow CancellationException (e.g., if (e is
CancellationException) throw e) before handling/logging fallback so coroutine
cancellation is preserved, similar to the inner fallback catch, and ensure the
same pattern is applied to any other broad Exception catches in
toggleStartRadioInternal that wrap suspend calls like
withContext(Dispatchers.IO) { ... } and radioQueue.getInitialStatus().
---
Nitpick comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 1844-1846: The closing braces after the catch rethrow in
MusicService.kt are squashed ("} }") and should be split onto separate
lines to match the project's brace style; locate the block that contains the
`throw e` (within the same method that parallels
`toggleLikeInternal`/`addToTargetPlaylistInternal`) and place each closing brace
on its own line so the method and enclosing block are clearly separated and
visually consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: acc71a80-a5f2-4f72-8837-c2673a9e6492
📒 Files selected for processing (1)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt
…ter failures Mirror CodeRabbit rollback handling in toggleEpisodeSaveForLater by restoring the previous DB row when remote saveEpisode sync fails, re-merging rolled-back metadata, refreshing notification and widget state, and rethrowing non-cancellation errors for centralized reporting.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt (3)
1444-1444: Avoid shadowing thecurrentMediaMetadatastate flow with a local.The class-level
currentMediaMetadata: MutableStateFlow<MediaMetadata?>(Line 305) is shadowed by a local val of the same name, which is easy to misread in a 100-line method that also touches the real flow elsewhere. Rename the local (e.g.playerMetadata) for clarity.- val currentMediaMetadata = player.currentMetadata ?: return - - val currentIndex = player.currentMediaItemIndex - val currentMediaId = currentMediaMetadata.id + val playerMetadata = player.currentMetadata ?: return + val currentIndex = player.currentMediaItemIndex + val currentMediaId = playerMetadata.id🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt` at line 1444, The method creates a local val named currentMediaMetadata that shadows the class-level MutableStateFlow currentMediaMetadata; rename the local to something like playerMetadata to avoid confusion. Specifically, replace the local declaration that assigns player.currentMetadata (e.g., val currentMediaMetadata = player.currentMetadata ?: return) with a distinct name (e.g., val playerMetadata = player.currentMetadata ?: return) and update all subsequent references in the method to use playerMetadata so the class property currentMediaMetadata remains unshadowed.
2058-2062: Inconsistent error handling vs. the other public toggles.
toggleLike/toggleLibrary/addToTargetPlaylistall wrap the internal suspend call intry { … } catch (CancellationException) { throw } catch (Exception) { log + reportException }.toggleStartRadiojust launches the internal underSilentHandler, so any non-cancellation failure (including the newIllegalStateException("No radio recommendations…")at Line 1539) is silently swallowed with no log. Either mirror the pattern used by the other toggles, or just delegate tostartRadioSeamlessly()which already has the wrapper.♻️ Consolidate on the existing wrapper
fun toggleStartRadio() { - scope.launch(SilentHandler) { - toggleStartRadioInternal() - } + startRadioSeamlessly() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt` around lines 2058 - 2062, toggleStartRadio currently launches toggleStartRadioInternal under SilentHandler and thus swallows non-cancellation exceptions; update it to follow the same error-handling pattern used by toggleLike/toggleLibrary/addToTargetPlaylist by wrapping the suspend call in try { ... } catch (CancellationException) { throw } catch (Exception e) { log the error and call reportException(e) }, or simply delegate from toggleStartRadio to startRadioSeamlessly() which already implements that wrapper; ensure you reference toggleStartRadio, toggleStartRadioInternal, and startRadioSeamlessly when making the change so the IllegalStateException("No radio recommendations…") and similar errors are properly logged and reported.
1786-1792: Dead-code fallback:currentSong.first()on a StateFlow is equivalent tocurrentSong.value.
currentSongis aStateFlow<Song?>(declared at Lines 306-310), andStateFlow.first()returns the current.valueimmediately — it does not wait for a non-null emission. SocurrentSong.value ?: currentSong.first() ?: return@withLockcollapses tocurrentSong.value ?: return@withLock. If the intent was to actually wait for a non-null song (useful right after session init), it needs a predicate.Same pattern repeats at Line 1863 (
toggleLikeInternal) and Line 1970 (addToTargetPlaylistInternal).♻️ Pick one of the two intents
If the fallback was never meant to wait, drop it:
- val songToToggle = currentSong.value ?: currentSong.first() ?: return@withLock + val songToToggle = currentSong.value ?: return@withLockIf the intent is to wait for a non-null song (e.g., toggle fired before the DB flow emitted), be explicit:
- val songToToggle = currentSong.value ?: currentSong.first() ?: return@withLock + val songToToggle = currentSong.value ?: currentSong.first { it != null } ?: return@withLockApply the same change at Lines 1863 and 1970.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt` around lines 1786 - 1792, The code uses currentSong.value ?: currentSong.first() which is redundant because StateFlow.first() returns the current value; decide one intent and update all three functions (toggleLibraryInternal, toggleLikeInternal, addToTargetPlaylistInternal): if you do not want to wait, remove the .first() fallback and use currentSong.value ?: return; if you want to wait for a non-null song, replace currentSong.first() with currentSong.first { it != null } and then unwrap the non-null (e.g., val songToToggle = currentSong.value ?: currentSong.first { it != null }!!.also { /* or safe cast */ }), making the same change in the corresponding lines for toggleLikeInternal and addToTargetPlaylistInternal so you either consistently use immediate .value or explicit first { it != null } to await a non-null emission.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 1423-1434: The current startRadioSeamlessly wrapper catches all
Exceptions and forwards them to reportException, which causes benign
IllegalStateException("No radio recommendations available for current track")
thrown by toggleStartRadioInternal to be reported; update startRadioSeamlessly
to special-case that expected failure by adding a specific catch for
IllegalStateException (or inspect the exception message) and handle it quietly
(e.g., debug/log + return) while preserving the existing catch for other
Exceptions that calls reportException; refer to startRadioSeamlessly,
toggleStartRadioInternal, and reportException when making the change.
---
Nitpick comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Line 1444: The method creates a local val named currentMediaMetadata that
shadows the class-level MutableStateFlow currentMediaMetadata; rename the local
to something like playerMetadata to avoid confusion. Specifically, replace the
local declaration that assigns player.currentMetadata (e.g., val
currentMediaMetadata = player.currentMetadata ?: return) with a distinct name
(e.g., val playerMetadata = player.currentMetadata ?: return) and update all
subsequent references in the method to use playerMetadata so the class property
currentMediaMetadata remains unshadowed.
- Around line 2058-2062: toggleStartRadio currently launches
toggleStartRadioInternal under SilentHandler and thus swallows non-cancellation
exceptions; update it to follow the same error-handling pattern used by
toggleLike/toggleLibrary/addToTargetPlaylist by wrapping the suspend call in try
{ ... } catch (CancellationException) { throw } catch (Exception e) { log the
error and call reportException(e) }, or simply delegate from toggleStartRadio to
startRadioSeamlessly() which already implements that wrapper; ensure you
reference toggleStartRadio, toggleStartRadioInternal, and startRadioSeamlessly
when making the change so the IllegalStateException("No radio recommendations…")
and similar errors are properly logged and reported.
- Around line 1786-1792: The code uses currentSong.value ?: currentSong.first()
which is redundant because StateFlow.first() returns the current value; decide
one intent and update all three functions (toggleLibraryInternal,
toggleLikeInternal, addToTargetPlaylistInternal): if you do not want to wait,
remove the .first() fallback and use currentSong.value ?: return; if you want to
wait for a non-null song, replace currentSong.first() with currentSong.first {
it != null } and then unwrap the non-null (e.g., val songToToggle =
currentSong.value ?: currentSong.first { it != null }!!.also { /* or safe cast
*/ }), making the same change in the corresponding lines for toggleLikeInternal
and addToTargetPlaylistInternal so you either consistently use immediate .value
or explicit first { it != null } to await a non-null emission.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: da5bd14a-4bdc-4152-aace-f2242505fd26
📒 Files selected for processing (1)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt
…or reporting Address CodeRabbit feedback by treating the expected no radio recommendations IllegalStateException in startRadioSeamlessly as a benign outcome, logging it at debug level instead of reporting it, while preserving reportException for unexpected IllegalStateException cases and other failures.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt (5)
2005-2063: Episode save-for-later now has rollback parity — matches the pattern used intoggleLibraryInternal/toggleLikeInternal.Previous-state capture, optimistic DB write, id-guarded in-memory metadata merge, immediate notification/widget refresh, and try/catch around
syncUtils.saveEpisodewith DB rollback + metadata re-merge on failure.CancellationExceptionis rethrown before rollback runs. Good.Very minor nit: Line 2013 uses fully-qualified
java.time.LocalDateTime.now()while the rest of the file (e.g. Lines 1803, 1887) uses the importedLocalDateTime.now(). Worth normalizing for consistency.✏️ Suggested diff
- val updatedInLibrary = if (isCurrentlySaved) null else java.time.LocalDateTime.now() + val updatedInLibrary = if (isCurrentlySaved) null else LocalDateTime.now()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt` around lines 2005 - 2063, The code in toggleEpisodeSaveForLater uses the fully-qualified java.time.LocalDateTime.now() (creating an inconsistency with the rest of the file); replace that usage with the imported LocalDateTime.now() to match existing style (ensure there's an import for java.time.LocalDateTime at the top if missing) so the timestamp assignment for updatedInLibrary follows the same import pattern as other uses like in toggleLibraryInternal / toggleLikeInternal.
1793-1853: Library toggle rollback pattern looks solid.Optimistic DB write → remote sync → DB re-read → id-guarded metadata merge → notification/widget refresh, with a symmetric rollback that re-reads the row after attempting to restore and rethrows non-cancellation exceptions. Cancellation is correctly rethrown before the rollback path as well. Nice.
Minor formatting nit on Line 1852 — the trailing
} }has collapsed braces with stray whitespace between them; not functional, just ugly when scanning diffs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt` around lines 1793 - 1853, Trailing collapsed braces and stray whitespace at the end of toggleLibraryInternal make the diff noisy; fix by normalizing the closing braces for the function and surrounding blocks (function toggleLibraryInternal and its inner withLock scope) so each closing brace is on its own line with standard indentation and no extra spaces between them.
1423-1549: Radio path: serialization + sentinel-based no-recommendations surfacing is clean.
toggleStartRadioInternalbeing gated bystartRadioMutexprevents concurrent queue mutation between the session-command path andstartRadioSeamlessly(),hasAppliedRadioItemscorrectly covers both the primary and fallback branches, and throwingIllegalStateExceptionwhen neither applies lets callers differentiate benign empty-results from real errors.startRadioSeamlesslynow narrows that case to a debug log (Lines 1429‑1435), matching the commit message's intent.One thing to watch: the sentinel is matched by message string (
"No radio recommendations available for current track"). If you touch this again, consider a dedicated private exception (e.g.private object NoRadioRecommendationsException : IllegalStateException(...)) to avoid the string-compare; purely optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt` around lines 1423 - 1549, The code currently signals "no recommendations" by throwing IllegalStateException with a specific message and startRadioSeamlessly checks the message string; change this to a dedicated exception type to avoid fragile string comparisons: define a private exception (e.g. private class NoRadioRecommendationsException : IllegalStateException(... ) or private object) and throw that from toggleStartRadioInternal instead of IllegalStateException("No radio recommendations available for current track"); then update startRadioSeamlessly to catch NoRadioRecommendationsException explicitly in its catch clauses (replacing the message equality check) while preserving the existing logging behavior and other exception handling.
1975-2003: Duplicate-insert guard and IO offload look correct; one tiny polish.Serializing under
addToTargetPlaylistMutex, readingAndroidAutoTargetPlaylistKeyon IO, the early-return for theTARGET_PLAYLIST_AUTOsentinel, and thecheckInPlaylist > 0guard beforeaddSongsToPlaylisttogether address rapid-tap duplicate inserts cleanly.Optional nit: the toast posted via
Handler(Looper.getMainLooper()).post { ... }at Lines 1984‑1991 could use the existingscopewithDispatchers.Main(orwithContext(Dispatchers.Main)) for consistency with the rest of the file, avoiding a one-offHandlerallocation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt` around lines 1975 - 2003, In MusicService.addToTargetPlaylistInternal, replace the Handler(Looper.getMainLooper()).post { Toast.makeText(...).show() } block with a coroutine Main dispatcher call (e.g. use withContext(Dispatchers.Main) { Toast.makeText(this@MusicService, getString(R.string.android_auto_target_playlist_not_set), Toast.LENGTH_SHORT).show() } or scope.launch(Dispatchers.Main) { ... }) so the toast runs on the main thread without allocating a one-off Handler; keep the exact message and target (this@MusicService, getString(...), Toast.LENGTH_SHORT) and leave the surrounding early-return logic unchanged.
681-696:distinctUntilChangedByover the relevant song fields is a nice replacement for the 1s debounce.Using
listOf(id, liked, inLibrary, title, thumbnailUrl, artists joined)as the dedupe key removes latency on favorite toggles while still avoiding spurious refreshes when unrelated fields change. Two tiny notes — neither a blocker:
- The
joinToString("|")key is O(n) per emission and theoretically collides if an artist name literally contains|. Realistically fine; if you ever want to be safe, aList<String>of artist names (orartists.map { it.name }) compares element-wise without a separator.- A small
data class NotificationKey(...)would read more self-documenting than an anonymouslistOf(...)and make future additions (e.g.isEpisode) harder to miss.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt` around lines 681 - 696, The current distinctUntilChangedBy key uses listOf(..., artists.joinToString("|")...) which is O(n) per emission and can collide if an artist name contains "|" — change the key to use a structural List of artist names (e.g. artists.map { it.name }) or, better, introduce a small data class (e.g. NotificationKey(id: String, liked: Boolean, inLibrary: Boolean, title: String?, thumbnailUrl: String?, artists: List<String>)) and use that as the dedupe key in currentSong.distinctUntilChangedBy; update usages in MusicService (the currentSong distinctUntilChangedBy block and the updateNotification/updateWidgetUI flow) to construct and return NotificationKey instead of the anonymous list to make comparisons element-wise and more self-documenting for future fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 2065-2069: toggleStartRadio() currently launches
toggleStartRadioInternal() under SilentHandler and swallows all exceptions;
change it to mirror startRadioSeamlessly(): call toggleStartRadioInternal()
inside a try/catch when launched from toggleStartRadio() (still using
SilentHandler), catch IllegalStateException with message "No radio
recommendations available for current track" and log it at debug/verbose, and
forward any other Throwable to reportException so real failures are reported;
optionally extract the common try/catch wrapper used by startRadioSeamlessly()
and toggleStartRadio() into a private helper (e.g. runStartRadioSafely) to avoid
duplicating the sentinel handling.
---
Nitpick comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 2005-2063: The code in toggleEpisodeSaveForLater uses the
fully-qualified java.time.LocalDateTime.now() (creating an inconsistency with
the rest of the file); replace that usage with the imported LocalDateTime.now()
to match existing style (ensure there's an import for java.time.LocalDateTime at
the top if missing) so the timestamp assignment for updatedInLibrary follows the
same import pattern as other uses like in toggleLibraryInternal /
toggleLikeInternal.
- Around line 1793-1853: Trailing collapsed braces and stray whitespace at the
end of toggleLibraryInternal make the diff noisy; fix by normalizing the closing
braces for the function and surrounding blocks (function toggleLibraryInternal
and its inner withLock scope) so each closing brace is on its own line with
standard indentation and no extra spaces between them.
- Around line 1423-1549: The code currently signals "no recommendations" by
throwing IllegalStateException with a specific message and startRadioSeamlessly
checks the message string; change this to a dedicated exception type to avoid
fragile string comparisons: define a private exception (e.g. private class
NoRadioRecommendationsException : IllegalStateException(... ) or private object)
and throw that from toggleStartRadioInternal instead of
IllegalStateException("No radio recommendations available for current track");
then update startRadioSeamlessly to catch NoRadioRecommendationsException
explicitly in its catch clauses (replacing the message equality check) while
preserving the existing logging behavior and other exception handling.
- Around line 1975-2003: In MusicService.addToTargetPlaylistInternal, replace
the Handler(Looper.getMainLooper()).post { Toast.makeText(...).show() } block
with a coroutine Main dispatcher call (e.g. use withContext(Dispatchers.Main) {
Toast.makeText(this@MusicService,
getString(R.string.android_auto_target_playlist_not_set),
Toast.LENGTH_SHORT).show() } or scope.launch(Dispatchers.Main) { ... }) so the
toast runs on the main thread without allocating a one-off Handler; keep the
exact message and target (this@MusicService, getString(...), Toast.LENGTH_SHORT)
and leave the surrounding early-return logic unchanged.
- Around line 681-696: The current distinctUntilChangedBy key uses listOf(...,
artists.joinToString("|")...) which is O(n) per emission and can collide if an
artist name contains "|" — change the key to use a structural List of artist
names (e.g. artists.map { it.name }) or, better, introduce a small data class
(e.g. NotificationKey(id: String, liked: Boolean, inLibrary: Boolean, title:
String?, thumbnailUrl: String?, artists: List<String>)) and use that as the
dedupe key in currentSong.distinctUntilChangedBy; update usages in MusicService
(the currentSong distinctUntilChangedBy block and the
updateNotification/updateWidgetUI flow) to construct and return NotificationKey
instead of the anonymous list to make comparisons element-wise and more
self-documenting for future fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: db516202-9890-47ba-9144-b519a73468ee
📒 Files selected for processing (1)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt
…ntry points Address CodeRabbit feedback by routing toggleStartRadio through the same guarded wrapper as startRadioSeamlessly, treating the expected no radio recommendations IllegalStateException as a debug-only outcome, and reporting unexpected failures through reportException while reusing a shared sentinel message constant.
…ng with consistent failure reporting and typed no-recommendation handling
…vent fallback on cancellation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt (2)
1543-1589:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-check the active track before applying fetched radio items.
currentMediaIdandcurrentIndexare captured before the IO work starts. If the user skips tracks or autoplay advances while the recommendation fetch is running, the laterremoveMediaItems/addMediaItems/queueTitle/currentQueueupdates are applied to whatever is playing now, not to the track that originally requested radio.Suggested fix
val currentIndex = player.currentMediaItemIndex val currentMediaId = currentMediaMetadata.id + fun isSameTrack() = + player.currentMetadata?.id == currentMediaId && + player.currentMediaItemIndex == currentIndex @@ if (initialStatus.title != null) { + if (!isSameTrack()) return@withLock queueTitle = initialStatus.title } @@ if (radioItems.isNotEmpty()) { + if (!isSameTrack()) return@withLock val itemCount = player.mediaItemCount @@ } - currentQueue = radioQueue + if (isSameTrack()) { + currentQueue = radioQueue + } } catch (e: Exception) { if (e is CancellationException) throw e // Fallback: try with related endpoint try { @@ if (radioItems.isNotEmpty()) { + if (!isSameTrack()) return@withLock val itemCount = player.mediaItemCountAlso applies to: 1596-1631
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt` around lines 1543 - 1589, The code captures currentMediaId/currentIndex before the network IO and then mutates the player's queue based on stale values; after the withContext(...) call and before doing queueTitle/currentQueue/removeMediaItems/addMediaItems/applyShuffleOrder, re-check the active track by comparing the saved currentMediaId/currentIndex to player.currentMetadata?.id and player.currentMediaItemIndex and abort (skip applying radio items) if they differ; apply the same guard for the later similar block (lines ~1596-1631) so you never modify the queue for a track that is no longer active.
1593-1640:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve real start-radio failures instead of always downgrading them to “no recommendations”.
If the primary fetch and the related-endpoint fallback both throw, both exceptions are swallowed and the function ends by throwing
NoRadioRecommendationsException().runStartRadioSafely()then treats that as a benign debug-only path, so real API/network failures are silently misclassified.Suggested fix
private suspend fun toggleStartRadioInternal() { startRadioMutex.withLock { + var lastFailure: Throwable? = null + // Safety Check: Ensure Player is initialized if (!playerInitialized.value) { Timber.tag(TAG).w("startRadioSeamlessly called before player initialization") return } @@ - } catch (e: Exception) { - if (e is CancellationException) throw e + } catch (e: Exception) { + if (e is CancellationException) throw e + lastFailure = e // Fallback: try with related endpoint try { ... - } catch (e: Exception) { - if (e is CancellationException) throw e - // No-op, will surface as command failure below. + } catch (e: Exception) { + if (e is CancellationException) throw e + lastFailure = e } } if (!hasAppliedRadioItems) { - throw NoRadioRecommendationsException() + throw (lastFailure ?: NoRadioRecommendationsException()) } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt` around lines 1593 - 1640, The current code swallows real failures from the primary YouTube fetch and the related-endpoint fallback and always throws NoRadioRecommendationsException(), losing the original error; modify the logic in runStartRadioSafely()/MusicService.kt so you capture the primary exception (e.g., save it in a variable like primaryError when catching the first Exception, rethrow immediately for CancellationException), then when the fallback catch runs if both attempts failed and hasAppliedRadioItems is false rethrow the captured primaryError (or the fallback error if primaryError is null) instead of always throwing NoRadioRecommendationsException(); keep NoRadioRecommendationsException() only for the case where both calls succeeded but returned no recommendations. Reference symbols: YouTube.next(WatchEndpoint), YouTube.related, hasAppliedRadioItems, primary catch blocks, and NoRadioRecommendationsException().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 1320-1328: The action label currently always toggles between
R.string.action_like and R.string.action_remove_like even for episode
save-for-later actions; update the setDisplayName(getString(...)) call in
MusicService.kt to pick episode-specific string resources when the item is an
episode (e.g., R.string.action_save_for_later and R.string.action_remove_saved)
instead of the like/unlike strings. Concretely, change the conditional passed to
getString (the expression using isFavorite) to first check the episode context
(e.g., isEpisode or the episode-aware state variable) and return the appropriate
episode strings, while keeping the existing heart icon logic (setIconResId)
as-is or adjust icon selection similarly if needed.
---
Outside diff comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 1543-1589: The code captures currentMediaId/currentIndex before
the network IO and then mutates the player's queue based on stale values; after
the withContext(...) call and before doing
queueTitle/currentQueue/removeMediaItems/addMediaItems/applyShuffleOrder,
re-check the active track by comparing the saved currentMediaId/currentIndex to
player.currentMetadata?.id and player.currentMediaItemIndex and abort (skip
applying radio items) if they differ; apply the same guard for the later similar
block (lines ~1596-1631) so you never modify the queue for a track that is no
longer active.
- Around line 1593-1640: The current code swallows real failures from the
primary YouTube fetch and the related-endpoint fallback and always throws
NoRadioRecommendationsException(), losing the original error; modify the logic
in runStartRadioSafely()/MusicService.kt so you capture the primary exception
(e.g., save it in a variable like primaryError when catching the first
Exception, rethrow immediately for CancellationException), then when the
fallback catch runs if both attempts failed and hasAppliedRadioItems is false
rethrow the captured primaryError (or the fallback error if primaryError is
null) instead of always throwing NoRadioRecommendationsException(); keep
NoRadioRecommendationsException() only for the case where both calls succeeded
but returned no recommendations. Reference symbols: YouTube.next(WatchEndpoint),
YouTube.related, hasAppliedRadioItems, primary catch blocks, and
NoRadioRecommendationsException().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b6edca48-1d3c-451d-86d1-f30eb80e6271
📒 Files selected for processing (1)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt (1)
4152-4174:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWidget heart isn't episode-aware, so episode "save for later" taps from the widget produce no visual feedback.
toggleLike()(line 4005, invoked fromMusicWidgetReceiver.ACTION_LIKE) now correctly delegates totoggleEpisodeSaveForLaterfor episodes, and both episode-toggle paths explicitly callupdateWidgetUI(player.isPlaying)(lines 2179, 2208). However,isLikedhere is derived solely fromsong?.liked, which is never set for episodes — saved episodes always render with the empty heart. Tapping the widget heart on a podcast then appears to do nothing, encouraging the user to tap again and toggleinLibraryback off.Mirror the
updateNotificationprecedence so the widget reflects episodeinLibrarystate for episodes:🛡️ Suggested fix
val songData = currentSong.value val song = songData?.song val songTitle = song?.title ?: getString(R.string.no_song_playing) val artistName = songData?.artists?.joinToString(", ") { it.name } ?: getString(R.string.tap_to_open) - val isLiked = songData?.song?.liked == true + val isLiked = + if (song?.isEpisode == true) { + song.inLibrary != null + } else { + song?.liked == true + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt` around lines 4152 - 4174, The widget heart currently reads only song?.liked, which doesn't reflect saved podcast episodes; change updateWidgetUI to derive isLiked by checking episode state first (mirror updateNotification precedence): e.g., compute isLiked = if (songData?.episode != null) (songData.episode.inLibrary == true) else (song?.liked == true), then pass that into widgetManager.updateWidgets; update the referenced symbols updateWidgetUI, currentSong, songData, song, song?.liked, songData.episode.inLibrary, and widgetManager.updateWidgets so episode saves show the filled heart.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 4152-4174: The widget heart currently reads only song?.liked,
which doesn't reflect saved podcast episodes; change updateWidgetUI to derive
isLiked by checking episode state first (mirror updateNotification precedence):
e.g., compute isLiked = if (songData?.episode != null)
(songData.episode.inLibrary == true) else (song?.liked == true), then pass that
into widgetManager.updateWidgets; update the referenced symbols updateWidgetUI,
currentSong, songData, song, song?.liked, songData.episode.inLibrary, and
widgetManager.updateWidgets so episode saves show the filled heart.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 311cce5f-3a90-4c32-8fbd-24108c04460a
📒 Files selected for processing (1)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt (1)
1565-1651:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
currentQueue(and possiblyqueueTitle) is mutated even when no radio items are applied, causing inconsistent state whenNoRadioRecommendationsExceptionis thrown.In the primary success branch, line 1601 sets
currentQueue = radioQueueregardless of whetherradioItemswas non-empty, and line 1577 may already have updatedqueueTitle. IfradioItemsis empty (e.g., the only result was the current track and got filtered out at line 1582-1584), the catch fallback is skipped, control falls through to line 1647-1649 and throwsNoRadioRecommendationsException. The caller (runStartRadioSafely) treats this as a benign no-op, but the service state is now:
currentQueue= a freshly-builtYouTubeQueuefor the radio with no items appended to the playerqueueTitle= the radio's title- player still contains the original items
Subsequent
onMediaItemTransitionauto-load-more (line 2575:currentQueue.nextPage()) will operate on this radio queue rather than the user's original queue.There's a symmetric gap on the catch fallback path: when the primary
trythrows and the fallback successfully appends items (line 1628),currentQueue = radioQueueis never reached, socurrentQueuekeeps pointing at the old queue while the player content reflects the radio.🛡️ Suggested precedence fix — only commit queue/title state when items actually got applied, in both branches
try { val initialStatus = withContext(Dispatchers.IO) { radioQueue .getInitialStatus() .filterExplicit(dataStore.get(HideExplicitKey, false)) .filterVideoSongs(dataStore.get(HideVideoSongsKey, false)) } - if (initialStatus.title != null) { - queueTitle = initialStatus.title - } - // Filter radio items to exclude current media item val radioItems = initialStatus.items.filter { item -> item.mediaId != currentMediaId } if (radioItems.isNotEmpty()) { val itemCount = player.mediaItemCount if (itemCount > currentIndex + 1) { player.removeMediaItems(currentIndex + 1, itemCount) } player.addMediaItems(currentIndex + 1, radioItems) hasAppliedRadioItems = true + if (initialStatus.title != null) { + queueTitle = initialStatus.title + } + currentQueue = radioQueue if (player.shuffleModeEnabled) { val shufflePlaylistFirst = dataStore.get(ShufflePlaylistFirstKey, false) applyShuffleOrder(player.currentMediaItemIndex, player.mediaItemCount, shufflePlaylistFirst) } } - - currentQueue = radioQueue } catch (e: Exception) { if (e is CancellationException) throw e // Fallback: try with related endpoint try { ... if (radioItems.isNotEmpty()) { ... player.addMediaItems(currentIndex + 1, radioItems) hasAppliedRadioItems = true + currentQueue = radioQueue if (player.shuffleModeEnabled) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt` around lines 1565 - 1651, The code sets queueTitle and currentQueue unconditionally which leads to inconsistent state when no radio items are applied; change logic so queueTitle and currentQueue are only mutated when hasAppliedRadioItems is true: move the assignment of queueTitle (currently done right after obtaining initialStatus) to occur only after radioItems were confirmed non-empty and added, and in the primary catch-fallback branch ensure that after adding items you also set currentQueue = radioQueue (and queueTitle if the fallback produced a title) before marking hasAppliedRadioItems true; keep throwing NoRadioRecommendationsException when hasAppliedRadioItems remains false. Use the existing symbols currentQueue, queueTitle, hasAppliedRadioItems, radioQueue, radioItems and NoRadioRecommendationsException to locate where to relocate these assignments.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 1565-1651: The code sets queueTitle and currentQueue
unconditionally which leads to inconsistent state when no radio items are
applied; change logic so queueTitle and currentQueue are only mutated when
hasAppliedRadioItems is true: move the assignment of queueTitle (currently done
right after obtaining initialStatus) to occur only after radioItems were
confirmed non-empty and added, and in the primary catch-fallback branch ensure
that after adding items you also set currentQueue = radioQueue (and queueTitle
if the fallback produced a title) before marking hasAppliedRadioItems true; keep
throwing NoRadioRecommendationsException when hasAppliedRadioItems remains
false. Use the existing symbols currentQueue, queueTitle, hasAppliedRadioItems,
radioQueue, radioItems and NoRadioRecommendationsException to locate where to
relocate these assignments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9bf5d22d-34e9-41d4-b096-cbd82b6436da
📒 Files selected for processing (1)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt (1)
1601-1649:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOnly switch
currentQueueafter radio items were actually applied.
currentQueue = radioQueueruns only in the primary path and before the finalhasAppliedRadioItemscheck. That means a no-results failure can still replace the queue source, while a fallback success leavescurrentQueuestale. Subsequent load-more/persistence logic will then operate on the wrong queue.🔧 Suggested fix
- currentQueue = radioQueue } catch (e: Exception) { if (e is CancellationException) throw e // Fallback: try with related endpoint try { @@ if (!hasAppliedRadioItems) { throw NoRadioRecommendationsException() } + + currentQueue = radioQueue } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt` around lines 1601 - 1649, The currentQueue assignment is happening before verifying radio items were actually applied, causing the queue source to be wrong on failures or fallbacks; move the assignment of currentQueue = radioQueue into the branches where radioItems are successfully added (i.e., inside the radioItems.isNotEmpty() block(s) after player.addMediaItems and after applyShuffleOrder logic), and do not set currentQueue anywhere if hasAppliedRadioItems remains false so the final throw NoRadioRecommendationsException() and subsequent load/persistence operate on the correct queue.
♻️ Duplicate comments (1)
app/src/main/kotlin/com/metrolist/music/playback/MediaLibrarySessionCallback.kt (1)
141-149:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't report the expected empty-radio result as an unknown error.
toggleStartRadioInternal()now uses a dedicated "no recommendations" failure for the normal empty-result path, but this catch still forwards it toreportException(...). That will turn a benign command failure from notification/Android Auto into error-noise. Special-case that sentinel and return aSessionErrorwithout reporting it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/kotlin/com/metrolist/music/playback/MediaLibrarySessionCallback.kt` around lines 141 - 149, In the catch (e: Exception) block in MediaLibrarySessionCallback.kt (around the handling of toggleStartRadioInternal()), special-case the "no recommendations" sentinel thrown by toggleStartRadioInternal() instead of treating it as an unknown error: detect the sentinel exception (the specific exception class or sentinel message/value that toggleStartRadioInternal() throws for the empty-radio path), and if it matches, return a SessionResult(SessionError(...)) directly without calling reportException(...); otherwise keep the existing reportException(e) and return the unknown-error SessionResult as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 1944-1948: currentSong is a StateFlow initialized with null so
using currentSong.value ?: currentSong.first() can return null immediately;
change the wait logic in the block that computes songToToggle (the variable
named songToToggle in MusicService.kt and the surrounding withLock) to use
currentSong.filterNotNull().first() so it suspends until a non-null song is
emitted (keep the fallback to reading from database.song(...) for previousSong
unchanged). Update any other similar occurrences (the ones around lines
referenced: usages near 2019-2023 and 2126-2130) to the same
filterNotNull().first() pattern to avoid no-op user actions.
---
Outside diff comments:
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt`:
- Around line 1601-1649: The currentQueue assignment is happening before
verifying radio items were actually applied, causing the queue source to be
wrong on failures or fallbacks; move the assignment of currentQueue = radioQueue
into the branches where radioItems are successfully added (i.e., inside the
radioItems.isNotEmpty() block(s) after player.addMediaItems and after
applyShuffleOrder logic), and do not set currentQueue anywhere if
hasAppliedRadioItems remains false so the final throw
NoRadioRecommendationsException() and subsequent load/persistence operate on the
correct queue.
---
Duplicate comments:
In
`@app/src/main/kotlin/com/metrolist/music/playback/MediaLibrarySessionCallback.kt`:
- Around line 141-149: In the catch (e: Exception) block in
MediaLibrarySessionCallback.kt (around the handling of
toggleStartRadioInternal()), special-case the "no recommendations" sentinel
thrown by toggleStartRadioInternal() instead of treating it as an unknown error:
detect the sentinel exception (the specific exception class or sentinel
message/value that toggleStartRadioInternal() throws for the empty-radio path),
and if it matches, return a SessionResult(SessionError(...)) directly without
calling reportException(...); otherwise keep the existing reportException(e) and
return the unknown-error SessionResult as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 685ae784-adba-4bb1-bb99-9974e9649cf3
📒 Files selected for processing (2)
app/src/main/kotlin/com/metrolist/music/playback/MediaLibrarySessionCallback.ktapp/src/main/kotlin/com/metrolist/music/playback/MusicService.kt
| val songToToggle = currentSong.value ?: currentSong.first() ?: return@withLock | ||
| val previousSong = | ||
| withContext(Dispatchers.IO) { | ||
| database.song(songToToggle.song.id).first()?.song | ||
| } ?: songToToggle.song |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the MusicService.kt file structure and find currentSong definition
cd app/src/main/kotlin/com/metrolist/music/playback && ls -laRepository: MetrolistGroup/Metrolist
Length of output: 793
🏁 Script executed:
# Get the file size first
wc -l app/src/main/kotlin/com/metrolist/music/playback/MusicService.ktRepository: MetrolistGroup/Metrolist
Length of output: 138
🏁 Script executed:
# Search for currentSong definition
rg "currentSong\s*=" app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt | head -20Repository: MetrolistGroup/Metrolist
Length of output: 159
🏁 Script executed:
# Also search for StateFlow and currentSong type
rg "StateFlow.*currentSong|currentSong.*StateFlow" app/src/main/kotlin/com/metrolist/music/playback/MusicService.ktRepository: MetrolistGroup/Metrolist
Length of output: 50
🏁 Script executed:
# Get the lines around 1944-1948
sed -n '1940,1950p' app/src/main/kotlin/com/metrolist/music/playback/MusicService.ktRepository: MetrolistGroup/Metrolist
Length of output: 502
🏁 Script executed:
# Get the lines around 2019-2023
sed -n '2015,2025p' app/src/main/kotlin/com/metrolist/music/playback/MusicService.ktRepository: MetrolistGroup/Metrolist
Length of output: 461
🏁 Script executed:
# Get the lines around 2126-2130
sed -n '2122,2132p' app/src/main/kotlin/com/metrolist/music/playback/MusicService.ktRepository: MetrolistGroup/Metrolist
Length of output: 550
🏁 Script executed:
# Search for currentSong property definition more broadly
rg "val currentSong|var currentSong" app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt -B 2 -A 2Repository: MetrolistGroup/Metrolist
Length of output: 750
🏁 Script executed:
# Check imports to see if filterNotNull is already imported
head -50 app/src/main/kotlin/com/metrolist/music/playback/MusicService.ktRepository: MetrolistGroup/Metrolist
Length of output: 1864
🏁 Script executed:
# Find the complete currentSong definition
rg "private val currentSong" app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt -A 15Repository: MetrolistGroup/Metrolist
Length of output: 685
🏁 Script executed:
# Search for stateIn to see if currentSong is wrapped with it
rg "stateIn|shareIn" app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt | grep -i currentRepository: MetrolistGroup/Metrolist
Length of output: 50
🏁 Script executed:
# Check imports related to Flow operations
grep -n "import.*flow\|import.*stateIn\|import.*filterNotNull" app/src/main/kotlin/com/metrolist/music/playback/MusicService.ktRepository: MetrolistGroup/Metrolist
Length of output: 590
🏁 Script executed:
# Get more context around currentSong definition (40 lines)
sed -n '1100,1200p' app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt | grep -n "currentSong\|stateIn\|flatMapLatest" -B 2 -A 2Repository: MetrolistGroup/Metrolist
Length of output: 50
🏁 Script executed:
# Find where currentSong property is defined more precisely
ast-grep --pattern 'val currentSong = $_'Repository: MetrolistGroup/Metrolist
Length of output: 853
🏁 Script executed:
# Search for filterNotNull usage in the file to see if it's already used
rg "filterNotNull" app/src/main/kotlin/com/metrolist/music/playback/MusicService.ktRepository: MetrolistGroup/Metrolist
Length of output: 50
🏁 Script executed:
# Check if filterNotNull is used in the broader codebase
rg "filterNotNull" app/src/main/kotlin/ | head -10Repository: MetrolistGroup/Metrolist
Length of output: 402
🏁 Script executed:
# Verify the StateFlow.first() semantics by looking at how currentSong is used elsewhere
rg "currentSong.first\(\)" app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt -B 2 -A 2Repository: MetrolistGroup/Metrolist
Length of output: 844
🏁 Script executed:
# Check the pattern with .value to understand the intended logic
rg "currentSong\.value.*currentSong\.first" app/src/main/kotlin/com/metrolist/music/playback/MusicService.ktRepository: MetrolistGroup/Metrolist
Length of output: 335
🏁 Script executed:
# Verify the imports one more time to confirm filterNotNull is NOT imported
grep "filterNotNull" app/src/main/kotlin/com/metrolist/music/playback/MusicService.ktRepository: MetrolistGroup/Metrolist
Length of output: 50
Use filterNotNull().first() to wait for the song instead of returning null immediately.
currentSong is a StateFlow initialized with null via stateIn(scope, SharingStarted.Lazily, null). Because StateFlow.first() returns the current cached value immediately, the pattern currentSong.value ?: currentSong.first() returns null without waiting when .value is null. This causes user actions to silently no-op during the post-track-change window while the database query is in flight.
Use filterNotNull().first() to wait for the next non-null emission instead:
🔧 Minimal fix
+import kotlinx.coroutines.flow.filterNotNull
...
- val songToToggle = currentSong.value ?: currentSong.first() ?: return@withLock
+ val songToToggle = currentSong.value ?: currentSong.filterNotNull().first()
...
- val songToToggle = currentSong.value ?: currentSong.first() ?: return@withLock
+ val songToToggle = currentSong.value ?: currentSong.filterNotNull().first()
...
- val currentSongItem = currentSong.value ?: currentSong.first() ?: return
+ val currentSongItem = currentSong.value ?: currentSong.filterNotNull().first()Also applies to: 2019-2023, 2126-2130
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/kotlin/com/metrolist/music/playback/MusicService.kt` around
lines 1944 - 1948, currentSong is a StateFlow initialized with null so using
currentSong.value ?: currentSong.first() can return null immediately; change the
wait logic in the block that computes songToToggle (the variable named
songToToggle in MusicService.kt and the surrounding withLock) to use
currentSong.filterNotNull().first() so it suspends until a non-null song is
emitted (keep the fallback to reading from database.song(...) for previousSong
unchanged). Update any other similar occurrences (the ones around lines
referenced: usages near 2019-2023 and 2126-2130) to the same
filterNotNull().first() pattern to avoid no-op user actions.
1 participant
Problem
Media notification behaviour was inconsistent and could feel laggy due to the 1 second debounce window. Podcast episodes also could not be visually liked from notification (heart state did not reliably reflect saved state). Notification action flows could further become inconsistent under rapid user interaction.
Cause
Custom media session commands were previously executed in a fire-and-forget style and returned success immediately. Playback actions could run concurrently without per-action serialization, which increased race risk for like, library, radio, and quick-add actions. Notification refresh depended on a debounce-based update path, and favourite state logic did not consistently apply episode semantics or immediate metadata refresh after episode save toggles.
Solution
inLibraryand song state toliked.Testing
./gradlew :app:assembleFossDebugRelated Issues
N/A
Summary by CodeRabbit
Bug Fixes
Refactor
Behavior